Skip to content

Conversation

mdedetrich
Copy link
Collaborator

The intention of this PR is to test if this project can compile with JDK 8. The reasons for this are

  1. At least with Pekko where we use this project to validate releases (i.e. we check if there is a bytecode difference between staging and locally built artifacts) and since Pekko is built with JDK 1.8 we constantly have to switch JDK's when testing with jardiff

  2. Given Add jardiff-sbt #52 it makes sense to target JDK 1.8 since almost all sbt plugins target 1.8 (given that its built with Scala 2.12).

@mdedetrich
Copy link
Collaborator Author

mdedetrich commented Oct 24, 2023

@retronym Do you by any chance know what the reason behind the JDK 11 requirement is? At least sbt test works fine with JDK 8 so I don't think there is a hard line technical JDK 8 requirement but I may be missing something?

@SethTisue
Copy link
Contributor

SethTisue commented Oct 26, 2023

#120 says something about logback? is that plausible? cc @Philippus

@mdedetrich
Copy link
Collaborator Author

#120 says something about logback? is that plausible? cc @Philippus

Anything against downgrading Logback to the latest one that supports 1.8? iirc That version of logback does work with newer versions assuming you just do standard logging.

@SethTisue
Copy link
Contributor

SethTisue commented Oct 26, 2023

No objection here, but that's just my ignorance on the subject talking. I'll try to rope in Jason.

@retronym
Copy link
Collaborator

I'm happy to widen support to JDK 8

@SethTisue SethTisue marked this pull request as draft October 29, 2023 14:43
@SethTisue
Copy link
Contributor

@mdedetrich okay, happy to merge a logback downgrade if you add one here. would you mind also adding a Scala Steward exemption so we don't accidentally re-upgrade it?

it would be nice to have some kind of smoke test for logging that would fail on JDK 8 without the upgrade, but I don't insist that you improve the status quo, there

@mdedetrich
Copy link
Collaborator Author

@mdedetrich okay, happy to merge a logback downgrade if you add one here. would you mind also adding a Scala Steward exemption so we don't accidentally re-upgrade it?

it would be nice to have some kind of smoke test for logging that would fail on JDK 8 without the upgrade, but I don't insist that you improve the status quo, there

I'll get onto this, will also do a smoke test

@som-snytt
Copy link

as a footnote, I would like to structure my utility to run on whichever JDK with whichever required dependencies. This came up with REPL and jline dropping JDK8. In the Age of Scala-CLI, this ought to be a non-issue. (Dropping JDK8 is not a simplification, because currently, one might want to support JDK11, 17, 21.)

@mdedetrich mdedetrich force-pushed the set-to-jdk-8 branch 4 times, most recently from 90ae75e to c180143 Compare October 30, 2023 07:42
@mdedetrich mdedetrich marked this pull request as ready for review October 30, 2023 07:43
@mdedetrich
Copy link
Collaborator Author

mdedetrich commented Oct 30, 2023

@SethTisue @retronym I have done everything that you have asked for. There were actually 2 dependencies I had to downgrade (logback/jgit) and a smoke test was added for both and I can confirm that this works locally by switching out JDK's. Scala steward ignore rules were also added.

Interestingly even though logback/slf4j is added as a dependency the project doesn't seem to do any actual logging (there may however be some indirect logging from transitive dependencies going on).

@SethTisue SethTisue merged commit 0c59ff1 into lightbend-labs:main Oct 31, 2023
@SethTisue
Copy link
Contributor

SethTisue commented Oct 31, 2023

thank you @mdedetrich !

Interestingly even though logback/slf4j is added as a dependency the project doesn't seem to do any actual logging

@retronym curious if you remember anything about that. slf4j is already there even in your initial commit.

@mdedetrich mdedetrich deleted the set-to-jdk-8 branch October 31, 2023 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants